prepared-metadata: harden concurrent cache safety - #947
Conversation
📝 WalkthroughWalkthroughThe change hardens prepared-result metadata caching against concurrent UDT, schema, and protocol decoder changes. Prepared statements now store frozen metadata, server metadata IDs, and decoder provenance atomically, while accessors return defensive copies. Protocol handler snapshots and synchronized UDT descriptor caches prevent decoder aliasing. Session and Sequence Diagram(s)sequenceDiagram
participant Session
participant PreparedStatement
participant ResponseFuture
participant ProtocolHandler
Session->>PreparedStatement: read metadata snapshot
Session->>ProtocolHandler: capture decoder context
Session->>ResponseFuture: submit EXECUTE
ResponseFuture->>ResponseFuture: select skip_meta
ResponseFuture->>ProtocolHandler: decode response
ResponseFuture->>PreparedStatement: publish refreshed metadata state
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Pull request overview
Hardens prepared-result metadata caching against concurrent schema, decoder, UDT-mapping, paging, and reprepare changes.
Changes:
- Atomically snapshots metadata, IDs, decoder provenance, and request state.
- Isolates and synchronizes UDT descriptor caching.
- Expands regression coverage and documents eligibility and compatibility behavior.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
cassandra/cluster.py |
Coordinates immutable decoder and request snapshots. |
cassandra/query.py |
Adds defensive metadata state and pickle migration. |
cassandra/protocol.py |
Isolates UDT descriptors and canonical decoder configuration. |
cassandra/cqltypes.py |
Makes UDT descriptor caching mapping-aware and thread-safe. |
tests/unit/test_response_future.py |
Tests paging, reprepare, decoder, and concurrency behavior. |
tests/unit/test_query.py |
Tests metadata immutability and serialization compatibility. |
tests/unit/test_prepared_metadata_cache_regressions.py |
Covers decoder and UDT-registration regressions. |
tests/unit/test_udt_descriptor_isolation.py |
Covers descriptor isolation and concurrent eviction. |
docs/scylla-specific.rst |
Documents metadata-cache eligibility and invalidation. |
CHANGELOG.rst |
Records hardening and compatibility changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cassandra/cluster.py (1)
5763-5786: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReturn after the reprepare ID mismatch instead of adopting the foreign response's metadata.
When
self.prepared_statement.query_id != response.query_id,_set_final_exceptionis called but execution falls through: lines 5782-5786 then publishresponse.column_metadata/response.result_metadata_idonto this statement's cached state and refresh the future's request snapshot from a PREPARE response that belongs to a different statement. The subsequentself._query(host)also re-sends a request on an already-failed future. The pre-existing fall-through was benign-ish; the new atomic_update_result_metadatamakes it corrupt the shared statement's metadata/id/provenance for every later execution.🐛 Proposed fix
if self.prepared_statement.query_id != response.query_id: self._set_final_exception(DriverException( "ID mismatch while trying to reprepare (expected {expected}, got {got}). " "This prepared statement won't work anymore. " "This usually happens when you run a 'USE...' " "query after the statement was prepared.".format( expected=hexlify(self.prepared_statement.query_id), got=hexlify(response.query_id) ) )) + return🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cassandra/cluster.py` around lines 5763 - 5786, In the reprepare response handling around self.prepared_statement, return immediately after _set_final_exception when the response.query_id differs from self.prepared_statement.query_id. This must prevent _update_result_metadata, _refresh_prepared_result_metadata, and the subsequent _query(host) from processing metadata belonging to a different statement; retain the existing metadata update path only for matching IDs.
🧹 Nitpick comments (3)
cassandra/cluster.py (1)
201-202: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider a
WeakKeyDictionaryfor the snapshot cache.
_PREPARED_METADATA_HANDLER_SNAPSHOTSholds strong references to handler classes (and their snapshot subclasses) forever. Eligible handlers are dynamically created classes in some cases (cython_protocol_handler()mints a freshCythonProtocolHandlerper call, each with its own_prepared_metadata_cache_token), so a long-lived process that builds handlers dynamically pins them permanently. A weak-keyed map keeps the same behavior without retaining otherwise-dead classes.♻️ Suggested change
-_PREPARED_METADATA_HANDLER_SNAPSHOTS = {} +_PREPARED_METADATA_HANDLER_SNAPSHOTS = WeakKeyDictionary() _PREPARED_METADATA_HANDLER_SNAPSHOT_LOCK = Lock()Note
weakrefis already imported in this module;WeakKeyDictionarywould need importing from it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cassandra/cluster.py` around lines 201 - 202, Replace _PREPARED_METADATA_HANDLER_SNAPSHOTS with a weak-keyed cache using weakref.WeakKeyDictionary, importing WeakKeyDictionary alongside the existing weakref import. Preserve _PREPARED_METADATA_HANDLER_SNAPSHOT_LOCK and all existing cache access behavior so dynamically created handler classes can be garbage-collected.tests/unit/test_prepared_metadata_cache_regressions.py (1)
164-178: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePositional index into the mocked
ResponseFuturecall is fragile.
call.args[1]depends on_create_response_futurecontinuing to pass(self, message, query, timeout)positionally. Asserting on the message via a named lookup (or at least a comment pinning the contract) would fail loudly rather than silently asserting on the wrong argument if that call is reordered.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_prepared_metadata_cache_regressions.py` around lines 164 - 178, Update the _capture_execute helper to retrieve the message argument from the mocked ResponseFuture call by its named parameter rather than relying on call.args[1]. Use the existing bound_result_metadata keyword lookup unchanged, and ensure the assertion targets the message passed by _create_response_future even if positional argument ordering changes.cassandra/protocol.py (1)
1143-1147: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated MRO-walk helper across modules.
_class_attributehere and_class_descriptorincassandra/cluster.py(lines 205-216) implement the same MRO lookup with slightly different internals (sentinel vs.None). Since the two results are compared against each other, keeping them as one shared helper would prevent them from drifting apart in a way that silently changes eligibility.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cassandra/protocol.py` around lines 1143 - 1147, Consolidate the duplicated MRO lookup by reusing one shared helper for both protocol.py’s _class_attribute and cluster.py’s _class_descriptor, including a consistent missing-attribute sentinel/return contract. Update both call sites so their comparisons preserve the existing eligibility behavior without separate implementations drifting.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cassandra/cluster.py`:
- Around line 5763-5786: In the reprepare response handling around
self.prepared_statement, return immediately after _set_final_exception when the
response.query_id differs from self.prepared_statement.query_id. This must
prevent _update_result_metadata, _refresh_prepared_result_metadata, and the
subsequent _query(host) from processing metadata belonging to a different
statement; retain the existing metadata update path only for matching IDs.
---
Nitpick comments:
In `@cassandra/cluster.py`:
- Around line 201-202: Replace _PREPARED_METADATA_HANDLER_SNAPSHOTS with a
weak-keyed cache using weakref.WeakKeyDictionary, importing WeakKeyDictionary
alongside the existing weakref import. Preserve
_PREPARED_METADATA_HANDLER_SNAPSHOT_LOCK and all existing cache access behavior
so dynamically created handler classes can be garbage-collected.
In `@cassandra/protocol.py`:
- Around line 1143-1147: Consolidate the duplicated MRO lookup by reusing one
shared helper for both protocol.py’s _class_attribute and cluster.py’s
_class_descriptor, including a consistent missing-attribute sentinel/return
contract. Update both call sites so their comparisons preserve the existing
eligibility behavior without separate implementations drifting.
In `@tests/unit/test_prepared_metadata_cache_regressions.py`:
- Around line 164-178: Update the _capture_execute helper to retrieve the
message argument from the mocked ResponseFuture call by its named parameter
rather than relying on call.args[1]. Use the existing bound_result_metadata
keyword lookup unchanged, and ensure the assertion targets the message passed by
_create_response_future even if positional argument ordering changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: aff8e12f-9d82-4741-92b1-2e5cacd29313
📒 Files selected for processing (10)
CHANGELOG.rstcassandra/cluster.pycassandra/cqltypes.pycassandra/protocol.pycassandra/query.pydocs/scylla-specific.rsttests/unit/test_prepared_metadata_cache_regressions.pytests/unit/test_query.pytests/unit/test_response_future.pytests/unit/test_udt_descriptor_isolation.py
f6cbf8d to
b844fee
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cassandra/protocol.py (1)
1150-1170: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider exposing a shared builder so the two config tuples cannot drift.
_prepared_metadata_cache_confighere and the inlinehandler_configtuple incassandra/cluster.py(lines 251-259) must stay structurally identical for eligibility comparison to work; any future edit to one (field order, added attribute) silently makes every handler ineligible forskip_metarather than failing loudly. A shared helper that returns the tuple from(handler, result_message_type, message_types, type_codes, code_to_type, decode_message, result_attributes, cep)— or at least a comment on both sides pointing at the other — would make the coupling explicit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cassandra/protocol.py` around lines 1150 - 1170, The configuration tuple construction is duplicated between _prepared_metadata_cache_config and cluster.py’s inline handler_config, allowing the structures to drift silently. Expose and reuse a shared builder accepting handler, result_message_type, message_types, type_codes, code_to_type, decode_message, result_attributes, and column_encryption_policy, then update both call sites to use it while preserving the existing tuple ordering and values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cassandra/protocol.py`:
- Around line 1150-1170: The configuration tuple construction is duplicated
between _prepared_metadata_cache_config and cluster.py’s inline handler_config,
allowing the structures to drift silently. Expose and reuse a shared builder
accepting handler, result_message_type, message_types, type_codes, code_to_type,
decode_message, result_attributes, and column_encryption_policy, then update
both call sites to use it while preserving the existing tuple ordering and
values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d12ce39b-87f3-4e53-83ae-643196096e8e
📒 Files selected for processing (10)
CHANGELOG.rstcassandra/cluster.pycassandra/cqltypes.pycassandra/protocol.pycassandra/query.pydocs/scylla-specific.rsttests/unit/test_prepared_metadata_cache_regressions.pytests/unit/test_query.pytests/unit/test_response_future.pytests/unit/test_udt_descriptor_isolation.py
b844fee to
cf09d2a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cassandra/cluster.py (1)
5029-5081: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the metadata/provenance resolution into a helper.
__init__now carries ~50 lines of two-branch skip-meta reconciliation logic that duplicates much of what_refresh_prepared_result_metadata()does. A small private helper (e.g._resolve_prepared_request_state(message, prepared_statement, bound_result_metadata, decoder_context)) returning(bound_metadata, decoder_context)would keep the constructor readable and make the two code paths easier to keep in sync.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cassandra/cluster.py` around lines 5029 - 5081, Extract the skip-meta reconciliation currently embedded in __init__ into a small private helper, such as _resolve_prepared_request_state, accepting the message, prepared statement, bound metadata, and decoder context and returning the resolved metadata/context pair. Preserve the existing handling for omitted snapshots, matching cached provenance, stale or unknown provenance, and message.skip_meta/result_metadata_id updates, then have __init__ use the helper before publishing _request_snapshot and align it with _refresh_prepared_result_metadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cassandra/cluster.py`:
- Around line 5029-5081: Extract the skip-meta reconciliation currently embedded
in __init__ into a small private helper, such as
_resolve_prepared_request_state, accepting the message, prepared statement,
bound metadata, and decoder context and returning the resolved metadata/context
pair. Preserve the existing handling for omitted snapshots, matching cached
provenance, stale or unknown provenance, and
message.skip_meta/result_metadata_id updates, then have __init__ use the helper
before publishing _request_snapshot and align it with
_refresh_prepared_result_metadata.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 77fbd33e-0ec6-4c6d-811e-81d405350d59
📒 Files selected for processing (10)
CHANGELOG.rstcassandra/cluster.pycassandra/cqltypes.pycassandra/protocol.pycassandra/query.pydocs/scylla-specific.rsttests/unit/test_prepared_metadata_cache_regressions.pytests/unit/test_query.pytests/unit/test_response_future.pytests/unit/test_udt_descriptor_isolation.py
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/scylla-specific.rst
- CHANGELOG.rst
nikagra
left a comment
There was a problem hiding this comment.
Thanks for picking this up so quickly after #770 — and for finding the bug class I missed. The diagnosis is right and it's the important part: the server's metadata_id describes only the server-side result shape, so a skip_meta response decoded with client-side decoding state that changed after PREPARE silently produces wrong Python objects. read_type mutating specialized_type.mapped_class on a process-global cached descriptor was a real cross-Cluster bug, and #770 shipped with it.
Reading the PR as four largely independent mechanisms:
PreparedStatement._result_metadata_state— the(metadata, id)pair becomes a 3-tuple adding decoder provenance; metadata frozen to tuples, defensive-copy getters, pickle migration.- Provenance token —
Cluster._prepared_metadata_context(rotated byregister_user_type) × a synthesized immutable snapshot of the protocol handler's decoder config. ResponseFuture._request_snapshot—(message, bound_metadata, decoder_context)published as one tuple, refreshed for paging and reprepare.- UDT descriptor isolation — cache keyed by
id(mapped_class)under a lock;read_typeno longer mutates a shared descriptor.
1, 3 and 4 I'd take as-is (modulo the inline nits). 2 splits in half: the Cluster-context part is a clean, cheap fix for the actual UDT bug. The handler-freezing part — synthesizing ResultMessage/ProtocolHandler subclasses and revalidating a 37-name attribute allowlist — is where I'd push back, on cost and on maintainability. Details in H1/H2 inline; my suggested alternative is at the bottom.
Performance
The handler snapshot is revalidated from scratch on every prepared-statement execute. _create_response_future passes self.client_protocol_handler — the source class — so handler_vars.get('_prepared_metadata_handler_snapshot') is never truthy and the early return never fires. _prepared_metadata_decoder_context additionally runs twice per future.
I pulled the four new functions out verbatim and measured them against stub classes shaped like the real ones:
per prepared-statement execute: 17.0 us
_prepared_metadata_protocol_handler_snapshot(): 12.6 us
_prepared_metadata_decoder_context(): 1.4 us
Stable across runs (16.7–17.8 µs). The ~12.6 µs is pure cache revalidation that a warm session recomputes identically every time: 3 dict copies, 37 _class_attribute MRO walks, two config tuples built and deep-compared, hash(cache_key), then the global lock and another deep compare.
Two caveats I want to be straight about. This isolates only the new code paths — it is not an end-to-end driver benchmark, and real per-query cost includes encoding, I/O and row decoding on top, so treat 17 µs as an absolute addition rather than a percentage. And lock contention is not the problem: tuple == short-circuits on identity, so hold time is negligible. The cost is CPU in the caller.
bench.py — no dependencies, python3 bench.py
"""Micro-benchmark of PR #947's per-query prepared-metadata bookkeeping.
Run with: python3 bench.py (no dependencies, no driver install needed)
_class_attribute, _prepared_metadata_cache_config,
_prepared_metadata_protocol_handler_snapshot and _prepared_metadata_decoder_context
are copied VERBATIM from the PR (cassandra/protocol.py, cassandra/cluster.py).
Only the surrounding driver classes are stubbed, shaped to match the real ones:
ResultMessage's full class-attribute set, FastResultMessage's code_to_type,
_ProtocolHandler / CythonProtocolHandler, 24 CQL type codes, 10 opcodes.
This isolates the cost of the new code paths only. It is NOT an end-to-end driver
benchmark -- real per-query cost includes encoding, I/O and row decoding on top.
"""
import timeit
from threading import Lock
from types import MappingProxyType
# ---------------------------------------------------------------- stub driver
class _MessageType(object):
tracing = False
custom_payload = None
warnings = None
# 24 CQL type codes, as in cassandra/type_codes.py
_TYPE_CODES = {i: type('T%d' % i, (), {}) for i in range(24)}
class ResultMessage(_MessageType):
"""Mirrors cassandra.protocol.ResultMessage's class-attribute surface."""
opcode = 0x08
name = 'RESULT'
kind = None
results = None
paging_state = None
type_codes = _cqltypes_by_code = _TYPE_CODES
_FLAGS_GLOBAL_TABLES_SPEC = 0x0001
_HAS_MORE_PAGES_FLAG = 0x0002
_NO_METADATA_FLAG = 0x0004
_CONTINUOUS_PAGING_FLAG = 0x40000000
_CONTINUOUS_PAGING_LAST_FLAG = 0x80000000
_METADATA_ID_FLAG = 0x0008
column_names = None
column_types = None
parsed_rows = None
continuous_paging_seq = None
continuous_paging_last = None
new_keyspace = None
column_metadata = None
query_id = None
bind_metadata = None
pk_indexes = None
schema_change_event = None
is_lwt = False
def __init__(self, kind):
self.kind = kind
def recv(self, *a): pass
@classmethod
def recv_body(cls, *a): pass
def recv_results_rows(self, *a): pass
def recv_results_prepared(self, *a): pass
def recv_results_metadata(self, *a): pass
def recv_prepared_metadata(self, *a): pass
def recv_results_schema_change(self, *a): pass
@classmethod
def read_type(cls, *a): pass
@staticmethod
def recv_row(*a): pass
class FastResultMessage(ResultMessage):
code_to_type = dict((v, k) for k, v in ResultMessage.type_codes.items())
def recv_results_rows(self, *a): pass
# 10 opcodes, as in _ProtocolHandler.message_types_by_opcode
_OPCODES = {i: type('M%d' % i, (), {}) for i in range(10)}
# ----------------------------- verbatim from PR: cassandra/protocol.py
_PREPARED_METADATA_RESULT_ATTRIBUTES = (
'opcode', 'name', 'kind', 'results', 'paging_state',
'_FLAGS_GLOBAL_TABLES_SPEC', '_HAS_MORE_PAGES_FLAG', '_NO_METADATA_FLAG',
'_CONTINUOUS_PAGING_FLAG', '_CONTINUOUS_PAGING_LAST_FLAG',
'_METADATA_ID_FLAG', 'column_names', 'column_types', 'parsed_rows',
'continuous_paging_seq', 'continuous_paging_last', 'new_keyspace',
'column_metadata', 'query_id', 'bind_metadata', 'pk_indexes',
'schema_change_event', 'is_lwt', 'tracing', 'custom_payload', 'warnings',
'__init__', 'recv', 'recv_body', 'recv_results_rows',
'recv_results_prepared', 'recv_results_metadata', 'recv_prepared_metadata',
'recv_results_schema_change', 'read_type', 'recv_row',
)
def _class_attribute(cls, name):
for base in cls.__mro__:
if name in vars(base):
return vars(base)[name]
return None
_PREPARED_METADATA_CONFIG_UNSET = object()
def _prepared_metadata_cache_config(
handler, result_message_type,
message_types=_PREPARED_METADATA_CONFIG_UNSET,
type_codes=_PREPARED_METADATA_CONFIG_UNSET,
code_to_type=_PREPARED_METADATA_CONFIG_UNSET,
decode_message=_PREPARED_METADATA_CONFIG_UNSET,
result_attributes=_PREPARED_METADATA_CONFIG_UNSET,
column_encryption_policy=_PREPARED_METADATA_CONFIG_UNSET):
message_types = (
handler.message_types_by_opcode
if message_types is _PREPARED_METADATA_CONFIG_UNSET else message_types)
type_codes = (
result_message_type.type_codes
if type_codes is _PREPARED_METADATA_CONFIG_UNSET else type_codes)
code_to_type = (
getattr(result_message_type, 'code_to_type', {})
if code_to_type is _PREPARED_METADATA_CONFIG_UNSET else code_to_type)
decode_message = (
_class_attribute(handler, 'decode_message')
if decode_message is _PREPARED_METADATA_CONFIG_UNSET else decode_message)
result_attributes = (
tuple((name, _class_attribute(result_message_type, name))
for name in _PREPARED_METADATA_RESULT_ATTRIBUTES)
if result_attributes is _PREPARED_METADATA_CONFIG_UNSET
else result_attributes)
column_encryption_policy = (
handler.column_encryption_policy
if column_encryption_policy is _PREPARED_METADATA_CONFIG_UNSET
else column_encryption_policy)
return (tuple(message_types.items()), result_message_type,
tuple(type_codes.items()), tuple(code_to_type.items()),
decode_message, result_attributes, column_encryption_policy)
class _ProtocolHandler(object):
message_types_by_opcode = _OPCODES
column_encryption_policy = None
_prepared_metadata_cache_token = object()
@classmethod
def decode_message(cls, *a): pass
_ProtocolHandler._prepared_metadata_cache_config = \
_prepared_metadata_cache_config(_ProtocolHandler, ResultMessage)
def cython_protocol_handler():
class CythonProtocolHandler(_ProtocolHandler):
my_opcodes = _ProtocolHandler.message_types_by_opcode.copy()
my_opcodes[FastResultMessage.opcode] = FastResultMessage
message_types_by_opcode = my_opcodes
_prepared_metadata_cache_token = object()
CythonProtocolHandler._prepared_metadata_cache_config = \
_prepared_metadata_cache_config(CythonProtocolHandler, FastResultMessage)
return CythonProtocolHandler
ProtocolHandler = cython_protocol_handler()
# ------------------------------ verbatim from PR: cassandra/cluster.py
_PREPARED_METADATA_HANDLER_SNAPSHOTS = {}
_PREPARED_METADATA_HANDLER_SNAPSHOT_LOCK = Lock()
def _callable_identity(value):
return getattr(value, '__func__', value)
def _prepared_metadata_protocol_handler_snapshot(protocol_handler):
try:
handler_vars = vars(protocol_handler)
except TypeError:
return protocol_handler
if handler_vars.get('_prepared_metadata_handler_snapshot'):
return protocol_handler
handler_token = handler_vars.get('_prepared_metadata_cache_token')
if handler_token is None:
return protocol_handler
try:
message_types = dict(protocol_handler.message_types_by_opcode)
result_message_type = message_types[ResultMessage.opcode]
if not isinstance(result_message_type, type):
return protocol_handler
type_codes = dict(getattr(result_message_type, 'type_codes', {}))
code_to_type = dict(getattr(result_message_type, 'code_to_type', {}))
decode_message = _class_attribute(protocol_handler, 'decode_message')
result_attributes = tuple(
(name, _class_attribute(result_message_type, name))
for name in _PREPARED_METADATA_RESULT_ATTRIBUTES
)
column_encryption_policy = getattr(
protocol_handler, 'column_encryption_policy', None)
handler_config = _prepared_metadata_cache_config(
protocol_handler, result_message_type, message_types, type_codes,
code_to_type, decode_message, result_attributes,
column_encryption_policy,
)
canonical_config = handler_vars.get('_prepared_metadata_cache_config')
if (canonical_config is None
or handler_config[:-1] != canonical_config[:-1]
or column_encryption_policy is not canonical_config[-1]):
return protocol_handler
cache_key = (protocol_handler, handler_token, handler_config[:-1],
id(column_encryption_policy))
hash(cache_key)
except Exception:
return protocol_handler
with _PREPARED_METADATA_HANDLER_SNAPSHOT_LOCK:
try:
cached = _PREPARED_METADATA_HANDLER_SNAPSHOTS.get(protocol_handler)
if cached is not None and cached[0] == cache_key:
return cached[1]
except Exception:
return protocol_handler
try:
result_attrs = dict(result_attributes)
result_attrs.update({
'__module__': result_message_type.__module__,
'type_codes': MappingProxyType(type_codes),
'_cqltypes_by_code': MappingProxyType(type_codes),
'code_to_type': MappingProxyType(code_to_type),
})
result_snapshot = type(
'_%sPreparedMetadataSnapshot' % result_message_type.__name__,
(result_message_type,), result_attrs)
message_types[ResultMessage.opcode] = result_snapshot
handler_attrs = {
'__module__': protocol_handler.__module__,
'message_types_by_opcode': MappingProxyType(message_types),
'column_encryption_policy': column_encryption_policy,
'_prepared_metadata_cache_token': object(),
'_prepared_metadata_handler_snapshot': True,
}
if decode_message is not None:
handler_attrs['decode_message'] = decode_message
snapshot = type(
'%sPreparedMetadataSnapshot' % protocol_handler.__name__,
(protocol_handler,), handler_attrs)
except Exception:
return protocol_handler
_PREPARED_METADATA_HANDLER_SNAPSHOTS[protocol_handler] = (
cache_key, snapshot)
return snapshot
def _prepared_metadata_decoder_context(protocol_handler, cluster_context):
if cluster_context is None:
return None
try:
handler_vars = vars(protocol_handler)
except TypeError:
return None
if not handler_vars.get('_prepared_metadata_handler_snapshot'):
return None
result_message_type = \
protocol_handler.message_types_by_opcode.get(ResultMessage.opcode)
type_codes = getattr(result_message_type, 'type_codes', {})
code_to_type = getattr(result_message_type, 'code_to_type', {})
column_encryption_policy = getattr(
protocol_handler, 'column_encryption_policy', None)
return (
handler_vars['_prepared_metadata_cache_token'],
protocol_handler,
cluster_context,
_callable_identity(protocol_handler.decode_message),
result_message_type,
_callable_identity(
getattr(result_message_type, 'recv_results_rows', None)),
_callable_identity(getattr(result_message_type, 'read_type', None)),
tuple(type_codes.items()),
tuple(code_to_type.items()),
column_encryption_policy,
id(column_encryption_policy),
)
# ------------------------------------------------------------------ benchmark
CLUSTER_CONTEXT = object()
# Warm the shared snapshot cache and produce a cached provenance token, i.e. the
# steady state of a Session that has already executed a prepared statement.
_SNAPSHOT = _prepared_metadata_protocol_handler_snapshot(ProtocolHandler)
CACHED_CTX = _prepared_metadata_decoder_context(_SNAPSHOT, CLUSTER_CONTEXT)
def per_query():
"""What one already-warm BoundStatement execute now costs.
Session._create_response_future passes the SOURCE handler class, so the
`_prepared_metadata_handler_snapshot` early return never fires and the full
revalidation runs again. ResponseFuture.__init__ then rebuilds the decoder
context a second time.
"""
# Session._create_response_future
handler = _prepared_metadata_protocol_handler_snapshot(ProtocolHandler)
ctx = _prepared_metadata_decoder_context(handler, CLUSTER_CONTEXT)
eligible = ctx is not None and CACHED_CTX == ctx
# ResponseFuture.__init__ (snapshot early-returns here; context is rebuilt)
handler2 = _prepared_metadata_protocol_handler_snapshot(handler)
ctx2 = _prepared_metadata_decoder_context(handler2, CLUSTER_CONTEXT)
return eligible and ctx == ctx2
assert per_query(), "steady state should be skip_meta-eligible"
N = 20000
print("per prepared-statement execute: %5.1f us"
% (timeit.timeit(per_query, number=N) / N * 1e6))
for label, fn in (
(" _prepared_metadata_protocol_handler_snapshot()",
lambda: _prepared_metadata_protocol_handler_snapshot(ProtocolHandler)),
(" _prepared_metadata_decoder_context()",
lambda: _prepared_metadata_decoder_context(_SNAPSHOT, CLUSTER_CONTEXT)),
):
print("%s: %5.1f us" % (label, timeit.timeit(fn, number=N) / N * 1e6))Test coverage
~1100 lines and the scenarios are well chosen — failed registration, the mid-registration transition window, in-place built-in map mutation, a non-subclassable result type, a raising __eq__, pickle migration, the no-op continuous-paging case. Asserting the snapshot doesn't call register_class is a nice touch. Gaps, none of which anchor to a single line:
- No completeness test for
_PREPARED_METADATA_RESULT_ATTRIBUTES— see H2. This is the one thing here most likely to rot silently, and it's the cheapest to guard. - No paging test.
start_fetching_next_page→_refresh_prepared_result_metadatais listed in the PR body ("coherent across … paging") but nothing exercises it, including the case where aMETADATA_CHANGEDon page 1 must change what page 2 sends. - No recovery test. The
Nonetransition context is covered; that a laterMETADATA_CHANGEDre-establishes an eligible context and turnsskip_metaback on is not — so a permanent-disable regression would pass CI. - No perf guard, given the above.
- Tests write module-global
_PREPARED_METADATA_HANDLER_SNAPSHOTSwithout cleanup (only the last usespatch.dict). Low risk since validation is content-based, but a fixture is cheaper than reasoning about it.
Security
Nothing new reaches the wire. The change actually closes a footgun rather than opening one: a process-global UDT descriptor mutated per response meant two Clusters with different registered classes for the same UDT could deserialize into each other's mapped class. No new deserialization surface; user-supplied __hash__/__eq__ are now invoked from driver code but are wrapped (see M2 on the silence).
Bottom line
Mechanisms 1, 3, 4 and the Cluster-context half of 2 are the fix. The handler-freezing half buys protection against runtime in-place mutation of ProtocolHandler.message_types_by_opcode — much rarer than the UDT case that motivated the PR — at the price of ~12.6 µs per execute, a hand-maintained allowlist that silently degrades as ResultMessage evolves, and ~250 lines of metaprogramming.
Would treating any non-canonical handler as ineligible by class identity alone work for you? Compare client_protocol_handler against the known-good set (_ProtocolHandler and the classes cython_protocol_handler() returns); anything else, or anything whose message_types_by_opcode is not the identical object it was at import, gets full metadata. No synthesized subclasses, no allowlist, no per-query revalidation — and the same safety for every case except "user mutated a built-in handler's maps in place and expects skip_meta to keep working", which I don't think we owe anyone.
Happy to be wrong on that if you have a concrete scenario in mind — mostly I want the cost and the allowlist maintenance burden to be a deliberate choice rather than a side effect.
| "Continuous paging backpressure is not supported.") | ||
|
|
||
|
|
||
| _PREPARED_METADATA_RESULT_ATTRIBUTES = ( |
There was a problem hiding this comment.
H2. This allowlist can't deliver the guarantee it's written for, and it will rot quietly.
The snapshot built from it is a subclass of the live result type (type('_%sPreparedMetadataSnapshot', (result_message_type,), result_attrs)), so any attribute not in this tuple still resolves dynamically through the mutable parent — and, more importantly, handler_config[:-1] != canonical_config[:-1] can't detect that such an attribute was mutated. A stale snapshot then keeps being handed out, which is the exact failure mode the machinery exists to prevent.
It's already incomplete: result_metadata_id is missing, though _METADATA_ID_FLAG is here and recv_results_metadata assigns self.result_metadata_id at protocol.py:816.
Second-order problem: _class_attribute returns None for a name absent from the whole MRO, and those Nones are then installed as class attributes on the snapshot via result_attrs. All 36 resolve today. The moment one is renamed, the snapshot silently gains X = None and an AttributeError at the mutation site becomes a TypeError deep inside decoding — e.g. flags & self._CONTINUOUS_PAGING_FLAG.
If the mechanism stays, please add a test asserting this tuple covers vars(ResultMessage) minus an explicit, documented exclusion set, so adding an attribute to ResultMessage breaks CI loudly instead of silently shrinking the guarantee. Right now nothing in the suite fails if this list falls behind.
There was a problem hiding this comment.
Addressed: removed the hand-maintained allowlist. Effective result attributes are now derived from the MRO, result_metadata_id is declared and frozen, and coverage verifies attributes remain frozen.
| - A prepared statement is eligible for ``skip_meta`` (metadata omitted from EXECUTE | ||
| responses) when it carries both a ``result_metadata_id`` and usable cached result | ||
| metadata from PREPARE, uses an unmodified built-in protocol handler, does not | ||
| supply the retained no-op ``continuous_paging_options`` compatibility argument, |
There was a problem hiding this comment.
M1b (docs). Two concerns with adding this bullet to the eligibility list:
- It surfaces an internal quirk to users.
continuous_paging_optionsisn't part of the documented public surface for Scylla users, so "don't pass the retained no-op compatibility argument" reads as a rule about something they've never heard of. - It inherits the "no-op" framing from the code comment, which isn't accurate — see H3 on
cluster.py.
Suggest dropping this bullet entirely; the mechanism is internal and the remaining conditions describe what users can actually act on.
| protocol_handler = \ | ||
| _prepared_metadata_protocol_handler_snapshot( | ||
| protocol_handler) | ||
| self._protocol_handler = protocol_handler |
There was a problem hiding this comment.
Nit — undocumented behavior change. Moving the handler assignment here (and dropping future._protocol_handler = self.client_protocol_handler from execute_async / execute_graph_async) changes what decodes PREPARE responses.
Session.prepare() previously built its ResponseFuture without setting _protocol_handler, so PREPARE decoded through the class default ProtocolHandler. It now decodes through client_protocol_handler. The comment at line 3467 argues that's the correct behavior and I agree — but it's user-visible for anyone with a custom handler, and it isn't in the CHANGELOG alongside the other Others entries.
Also worth a note: _target_analytics_master now passes protocol_handler=ProtocolHandler explicitly (line 3182), which is what preserves the old behavior for that internal path. Good catch, but it reads as arbitrary without a comment saying it's deliberate — a future cleanup will "fix" it to client_protocol_handler.
There was a problem hiding this comment.
Added the custom PREPARE-handler behavior to CHANGELOG and documented why the analytics-master query deliberately retains ProtocolHandler.
cf09d2a to
221daae
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/scylla-specific.rst`:
- Around line 217-222: Revise the documentation around the statement-type
eligibility claim so it says statement type is unrestricted only when the
documented result-metadata conditions are met. Explicitly preserve the
exclusions for custom handlers, modified built-in configurations, and other
contexts that receive full metadata, rather than implying that any prepared
statement qualifies.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 33a88a4a-9919-46d3-948c-e6ac2970481a
📒 Files selected for processing (11)
CHANGELOG.rstcassandra/cluster.pycassandra/cqltypes.pycassandra/protocol.pycassandra/query.pydocs/scylla-specific.rsttests/unit/test_prepared_metadata_cache_regressions.pytests/unit/test_protocol.pytests/unit/test_query.pytests/unit/test_response_future.pytests/unit/test_udt_descriptor_isolation.py
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.rst
| non-empty cached result columns. This includes SELECT queries and conditional | ||
| INSERT, UPDATE, and DELETE statements (lightweight transactions), whose results | ||
| include an ``[applied]`` column and may include values from the existing row. | ||
| Ordinary DML statements that produce no result columns remain ineligible. There is | ||
| no code-level restriction by statement type; the behaviour follows directly from | ||
| the result metadata. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Avoid overstating skip_meta eligibility.
“Any prepared statement” conflicts with the eligibility requirements above: custom handlers, modified built-in configurations, and other ineligible execution contexts still receive full metadata. Clarify that statement type is unrestricted, but the documented eligibility conditions still apply.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/scylla-specific.rst` around lines 217 - 222, Revise the documentation
around the statement-type eligibility claim so it says statement type is
unrestricted only when the documented result-metadata conditions are met.
Explicitly preserve the exclusions for custom handlers, modified built-in
configurations, and other contexts that receive full metadata, rather than
implying that any prepared statement qualifies.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
cassandra/cluster.py:3269
- Defaulting this private factory to
session.client_protocol_handleralso changes internal callers.QueryTrace._executecalls_create_response_futuredirectly (cassandra/query.py:1239), so trace metadata queries now use an application-custom handler even thoughSession.client_protocol_handleris documented for client-initiated requests only (cassandra/cluster.py:2902-2911). A handler that changes result decoding can therefore break trace retrieval. PreserveProtocolHandleras the factory default and pass the client handler explicitly from the public execute paths, or explicitly opt the trace caller out as done for the analytics-master query.
if protocol_handler is _NOT_SET:
protocol_handler = self.client_protocol_handler
Summary
Follow-up to #770, which introduced
SCYLLA_USE_METADATA_IDsupport.This hardens prepared-result metadata caching so
skip_metacannot pair a fresh server metadata id with stale column definitions or a different client decoder configuration.What changed
continuous_paging_optionsbehavior while conservatively disablingskip_metawhen it is supplied.Compatibility / risk
PreparedStatement.result_metadataandresult_metadata_idremain read-only as introduced in #770. Code needing to replace them must useupdate_result_metadata(metadata, metadata_id). The metadata getter still returns a list, now as a defensive copy.The wire layout is unchanged outside the already-negotiated metadata-id extension. Custom protocol handlers receive full result metadata rather than using the optimization.
Validation
TZ=UTC uv run pytest -rf tests/unit— 795 passed, 45 skippedgit diff --check— clean